-
Notifications
You must be signed in to change notification settings - Fork 169
Add some missing tests #392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@cuviper Hello! I was wondering if there's any feedback on this PR. I've ensured all checks are passing and I'm happy to address any concerns or make changes as needed. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few places where I suggest changes that would apply elsewhere as well, so please do. (manually or by LLM is up to you)
@cuviper Thanks for your correction and thanks for your time to review my PR!😊 All feedback has been addressed, and I've marked the relevant comments as resolved. Please let me know if there are any further questions! |
I will review again when I get a chance, but please stop pinging so much. You've repeated nearly the same comment now 4 times, which I guess you deleted and reposted. I see no urgency just for test coverage. |
Hi cuviper, I owe you a sincere apology for the multiple pings earlier. When I saw your activity but no review progress for this pr, I mistakenly assumed you might have missed my updates — especially since I deleted and reposted comments thinking it would refresh the notification for you. I now realize this created four disruptive pings, and I fully understand it was both unnecessary and disrespectful of your time. I realize it must have made you feel pushed or even overwhelmed. That was never my intention, and I’m truly sorry for the annoyance. Your latest feedback has been fully addressed in the new commit. I’d appreciate your review whenever it’s convenient for you. Thank you for your patience and for teaching me so much.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing, then I think we're good!
let result = set.shift_take(&2); | ||
assert_eq!(result, Some(2)); | ||
|
||
set.swap_remove(&3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's one more swap in a shift test, please update it like the others.
Hi,
Thanks for your time to review this PR.
By examining the existing code, we found that some tests can be added to improve the repo's overall test coverage. We've added some tests to cover missing unit tests in slice.rs, map.rs and set.rs. The tests we submitted have been carefully curated by us to ensure their behavior and effectiveness.
Thanks again for reviewing.